fix #93316: slur still lays out to deleted note #2343
Conversation
|
Test failure might be real, but I suspect it will turn out to be a case of the reference expecting the buggy result. |
|
Failure was real; the splitstaff function needed to be updated to fix up slurs before deleting the original note rather than after. |
| // spanners with this cr as start or end element will need relayout | ||
| SpannerMap& smap = score()->spannerMap(); | ||
| auto spanners = smap.findOverlapping(tick(), tick()); | ||
| for (auto i = spanners.begin(); i < spanners.end(); i++) { |
There was a problem hiding this comment.
I would use auto everywhere.
auto spanners = smap.findOverlapping(tick(), tick());
for (auto interval : spanners) {
|
Looks good. Could you harmonize the calls to findOverlapping() with auto? Also in other files and squash the commits together? Thanks! |
|
OK, will do. "auto" is relatively new to me so I've never been clear on how it works or when it is most appropriate to use, but I think I get it. |
MarcSabatella
force-pushed
the
93316-slur-relayout
branch
from
08ff999
to
45e6e2d
Compare
|
Done, and tested each case to be sure they worked. BTW, I noted sometimes we used constructions like: for (auto interval : findOverlapping(tick, tick)) with the call to findOverlapping within the "for" loop. Some of these were "foreach" loops. I'm not really sure if that gets re-evaluated every time throug, and I'm also not sure if any of these call mights have problems with removing elements from the listed being iterated over. I decided to standardize all of these to look more like: auto spanners = findOverlapping(tick, tick); |
|
As far as I understand we don't remove the spanner from the map we iterate on. So we should be fine. |
fix #93316: slur still lays out to deleted note
| @@ -446,7 +446,8 @@ bool Score::rewriteMeasures(Measure* fm, Measure* lm, const Fraction& ns, int st | |||
|
|
|||
| int tick1 = m1->tick(); | |||
| int tick2 = m2->endTick(); | |||
| for (auto i : s->spannerMap().findContained(tick1, tick2)) | |||
| auto spanners = s->spannerMap().findContained(tick1, tick2); | |||
| for (auto i : spanners) | |||
| undo(new RemoveElement(i.value)); | |||
There was a problem hiding this comment.
FWI, this was what I was concerned about when I thought maybe we might be removing from the list we were iterating on, depending on exactly how those loops are evaluated. Probably there was never a problem because either findContained() is re-evaluated on each iteration, or the result of findContained() was stored in a temporary variable and removing from the score might be removed from the spanner map but the temporary variable wouldn't be affected. Still it seemed awkward, so I just made this look like the others, first calling findContained() and then initiating the loop.
Same story with the call in joinMeasures() a little farther down.
This code works and should be safe to the extent I understand the spanner map implementation, which is not very well. Main question to me is if there is a better / more direct way to find the spanners attached to this chordrest. I borrowed this code from ChordRest::accessibleExtraInfo().